Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for armv7l. #253

Merged
merged 6 commits into from
Dec 11, 2024
Merged

Add support for armv7l. #253

merged 6 commits into from
Dec 11, 2024

Conversation

jsirois
Copy link
Collaborator

@jsirois jsirois commented Dec 11, 2024

No description provided.

@jsirois jsirois force-pushed the armv7l branch 2 times, most recently from 8686ecc to 5d881ff Compare December 11, 2024 00:39
@jsirois
Copy link
Collaborator Author

jsirois commented Dec 11, 2024

@Lauszus thanks for getting the ball rolling in a-scie/jump#256.
I think this approach using cross is probably a bit cleaner, but I'd like your feedback. In particular, unlike in your change, I'm just rolling with env::consts::ARCH here for armv7l which means the binary suffix for arm 32 is just arm. I'd like to make sure we agree on a suffix, because I'll want ptex, scie-jump and science to all use the same one.

@jsirois jsirois mentioned this pull request Dec 11, 2024
Merged
jsirois
jsirois commented Dec 11, 2024
View reviewed changes
.github/workflows/ci.yml Outdated
Comment on lines 35 to 41
cross-target: x86_64-unknown-linux-musl
- os: ubuntu-24.04
name: Linux aarch64
cross-target: aarch64-unknown-linux-musl
- os: ubuntu-24.04
name: Linux armv7l
cross-target: armv7-unknown-linux-musleabihf
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Lauszus note that no cargo config is needed for linker flags, just specifying the musl targets is enough to get you a statically linked executable:

:; file dist/ptex-linux-* | grep -v sha256
dist/ptex-linux-aarch64:        ELF 64-bit LSB executable, ARM aarch64, version 1 (SYSV), statically linked, stripped
dist/ptex-linux-arm:            ELF 32-bit LSB executable, ARM, EABI5 version 1 (SYSV), statically linked, stripped
dist/ptex-linux-x86_64:         ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV), static-pie linked, stripped

I built those on my GNU-Linux x86-64 box using cross run -p package --target {x86_64-unknown-linux-musl,aarch64-unknown-linux-musl,armv7-unknown-linux-musleabihf}.

benjyw
benjyw approved these changes Dec 11, 2024
View reviewed changes
Copy link
Collaborator

@benjyw benjyw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, about as streamlined as I can imagine it being. And I concur that cross-rs is likely the way to go (and it looks like they officially bless tonistiigi/binfmt...)

@Lauszus
Copy link
Contributor

Lauszus commented Dec 11, 2024

@Lauszus thanks for getting the ball rolling in a-scie/jump#256. I think this approach using cross is probably a bit cleaner, but I'd like your feedback. In particular, unlike in your change, I'm just rolling with env::consts::ARCH here for armv7l which means the binary suffix for arm 32 is just arm. I'd like to make sure we agree on a suffix, because I'll want ptex, scie-jump and science to all use the same one.

Yeah that is better. I will update my PR to use cross as well.

I don't think we should use arm for the executable, as it is very ambiguous. Anything else I have ever seen either uses armv7l or aarch64. This is also what Python standalone uses and what is used for Raspberry Pi wheels.

Lauszus
Lauszus reviewed Dec 11, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
Lauszus
Lauszus reviewed Dec 11, 2024
View reviewed changes
.github/workflows/ci.yml
if: matrix.cross-target != ''
run: |
cargo install cross --locked
docker run --privileged --rm tonistiigi/binfmt --install all
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
docker run --privileged --rm tonistiigi/binfmt --install all

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this line really needed?

Copy link
Collaborator Author

@jsirois jsirois Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Without it, lint works (cross clippy), but not test or package (cross test or cross run -p package). Although the cross images contain a cross-compiler toolchain for each target, that's only 1/2 the battle for commands like cargo test and cargo run, the other 1/2 is actually running the binary that is compiled and that requires qemu / binfmt support.

Lauszus
Lauszus reviewed Dec 11, 2024
View reviewed changes
CHANGES.md Outdated Show resolved Hide resolved
jsirois and others added 3 commits December 11, 2024 07:29
@jsirois @Lauszus
Update README.md
5787e50 
Co-authored-by: Kristian Sloth Lauszus <[email protected]>
Signed-off-by: John Sirois <[email protected]>
@jsirois @Lauszus
Update CHANGES.md
f47c7bd 
Co-authored-by: Kristian Sloth Lauszus <[email protected]>
Signed-off-by: John Sirois <[email protected]>
@jsirois
Use custom armv7l binary suffix.
bb07d22
@jsirois
Copy link
Collaborator Author

jsirois commented Dec 11, 2024

I don't think we should use arm for the executable, as it is very ambiguous. Anything else I have ever seen either uses armv7l or aarch64. This is also what Python standalone uses and what is used for Raspberry Pi wheels.

Ok, fixed up to use armv7l like you did in the scie jump.

If this goes green I'll merge and test out the release process. If that goes well, I'll swing back to your similar scie jump change and get that released and then work on science and then Pex.

Thanks again @Lauszus.

@jsirois jsirois merged commit 0f50b3f into a-scie:main Dec 11, 2024
9 checks passed
@jsirois jsirois deleted the armv7l branch December 11, 2024 16:21
@Lauszus
Copy link
Contributor

Lauszus commented Dec 11, 2024

I don't think we should use arm for the executable, as it is very ambiguous. Anything else I have ever seen either uses armv7l or aarch64. This is also what Python standalone uses and what is used for Raspberry Pi wheels.

Ok, fixed up to use armv7l like you did in the scie jump.

If this goes green I'll merge and test out the release process. If that goes well, I'll swing back to your similar scie jump change and get that released and then work on science and then Pex.

Thanks again @Lauszus.

You are welcome. I can send an initial PR for pex and science if you want, as I started on it, but then stopped as I was waiting for a new jump and ptex release.

@jsirois
Copy link
Collaborator Author

jsirois commented Dec 11, 2024

I can send an initial PR for pex and science if you want, as I started on it, but then stopped as I was waiting for a new jump and ptex release.

That sounds great.

@Lauszus
Copy link
Contributor

Lauszus commented Dec 11, 2024

I can send an initial PR for pex and science if you want, as I started on it, but then stopped as I was waiting for a new jump and ptex release.

That sounds great.

I opened two draft PRs: a-scie/lift#106 and pex-tool/pex#2620. I'll just clean them up a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lauszus Lauszus Lauszus left review comments

@benjyw benjyw benjyw approved these changes

@huonw huonw Awaiting requested review from huonw

@sureshjoshi sureshjoshi Awaiting requested review from sureshjoshi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jsirois @Lauszus @benjyw